Skip to content

Merge Task Retry Handling & Performance - #860

Draft
EvanDietzMorris wants to merge 9 commits into
masterfrom
task-retry-performance
Draft

Merge Task Retry Handling & Performance#860
EvanDietzMorris wants to merge 9 commits into
masterfrom
task-retry-performance

Conversation

@EvanDietzMorris

@EvanDietzMorris EvanDietzMorris commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This PR aims to improve performance, resource utilization, and error reporting for the merge_and_post_process celery task queue implementation.

Issues Addressed
Currently, merge_and_post_process celery tasks are retried for three broad reasons: a failure to acquire an 'expensive token', failure to acquire a merge db lock, or when any Exception is raised during merge_and_post_process. All three of these share the same retry implementation, retry counter, and an exponential back off behavior that delays the next attempt by increasing amounts. The exponential back off delay is tuned to be quite slow, in an attempt to give downstream services time to recover in event of an error, but for normal 'expensive token' or db lock contention this isn't appropriate and results in unnecessary downtime when long back off delays occur while resources might already be available. Additionally, if deterministic errors occur, the current implementation still retries them up to the shared limit of retries even though they will fail again, wasting time and resources, with long delays in between.

The Solution
This PR separates the retry logic for Exceptions from retries due to contention, allowing us to tune them differently. By default it attempts retries due to contention much faster and for more attempts (every 2-5 seconds up to 100 times) and retries due to exceptions with the existing exponential back off (1, 2, 4, 8, 16s, then capped at 30s) for 8 attempts (about two minutes of total patience), enough for the backoff tail to actually reach the 30s cap and outlast an annotator/appraiser pod restart or a rolling deploy.

DB Lock Silent Failures
Here we also address a mismatch for how db lock retries were handled vs token contention. Previously the db lock had hard coded retry values inside the merge_and_post_process function (with a cap of 10 instead of 20), and when it was exhausted and gave up it silently caught the error with only a log message. Now the db lock contention retry shares the implementation of the token contention retry, and raises the failure when it occurs, allowing Celery and OTEL to recognize it. In addition, when the contention retry budget is exhausted, subscribers now receive a merged_version_failed notification so the failure is visible to API consumers rather than only in logs and traces.

New ENV Vars
This introduces two new env vars for tuning this retry logic, but does not attempt to alter the helm chart due to settings being applied in jenkins outside of this repo. That should be addressed in future work.

ARS_EXPENSIVE_TASK_MAX_RETRIES (the overall shared contention retry limit, default 100)
ARS_MERGE_ERROR_MAX_RETRIES (retry limit for real errors, default 8)

Tests
tests/unit/test_merge_retry.py adds unit coverage for the retry semantics with the DB, redis gate, and merge internals mocked out.

Misc.
This also changes a logging call in expensive_gate indicating a token acquisition is being attempted from warning to debug, to avoid clogging up logs and unnecessary log writes, because this is a normal operation that should happen all the time, and more frequently with these changes. The REDIS_URL (which may carry credentials) was also removed from that log line.

Future work
It is important to note that there are still significant improvements that could be made for error handling here. All exceptions are handled in the same way. Except for other logs outside of merge_and_post_process it is not apparent what kind of issue caused Exceptions or retries for them, which still causes deterministic errors to be retried unnecessarily, and hurts our ability to diagnose issues. Perhaps more importantly, according to Claude, errors that occur when hitting the node annotator or appraiser are swallowed and don't even cause Exceptions to be raised into the merge_and_post_process - so at least some of the kinds of errors that it is most appropriate to retry with exponential back off on probably aren't even being retried.

Two further limitations are known and deliberately deferred to follow-up PRs rather than addressed here:

  • Token contention and merge-lock contention still share one retry counter (self.request.retries) against the same ARS_EXPENSIVE_TASK_MAX_RETRIES budget, so heavy token contention reduces the budget left for lock contention.
  • Task payloads carry the full TRAPI message, and every retry republishes it to the broker. A follow-up will pass the child message PK instead and load from the DB inside the task, after which a subsequent PR can replace the redis token gate with a dedicated bounded-concurrency celery queue, eliminating contention retries (and both env vars) entirely.

- separate retry functionality for errors from retries due to contention (failure to acquire a token or a db lock)
- allow different max retry settings for each
- make the backoff timing for normal retries constant/faster vs the exponential backoff for real errors
- fix silent handling of merge lock retry exhaustion
@EvanDietzMorris
EvanDietzMorris marked this pull request as draft September 4, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant